Fix setState regression #1771

oliviertassinari · 2018-08-18T15:27:47Z

I have noticed the following regression after upgrade to enzyme 3.4.4.

TypeError: ReactWrapper::setState() expects a function as its second argument

I think that we are closer to how React behaves this way: https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L77-L92

I have noticed the following regression after upgrade to enzyme 3.4.4. > TypeError: ReactWrapper::setState() expects a function as its second argument I think that we are closer to how React behaves this way: https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberReconciler.js#L144-L153.

oliviertassinari · 2018-08-18T15:30:37Z

Well, my fix is probably wrong, I don't have the time to look deeper, but at least you get the idea. This is fine with React, I don't think that enzyme should throw.

this.setState({}, null); // OK
this.setState({}, undefined); // OK
this.setState({}, () => {}); // OK
this.setState({}, {}); // NOT OK

ljharb

Why shouldn't it throw? If you're doing that in React, it's probably a bug. That React chooses not to throw is fine, but enzyme is a test framework - its job is to catch bugs.

Do you have a legitimate use case for passing a falsy non-function to either setState or setProps?

ljharb · 2018-08-18T16:55:44Z

packages/enzyme/src/ShallowWrapper.js

    if (this[ROOT] !== this) {
      throw new Error('ShallowWrapper::setProps() can only be called on the root');
    }
-    if (typeof callback !== 'function') {
+    if (callback && typeof callback !== 'function') {


if you pass undefined, the current code will replace it with noop - this would allow you to silently pass a falsy primitive (false, NaN, 0, empty string, null). What's the utility of this?

What about null? The default won't kick in. React checks both:
https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L179

function b(b = 'b') { return b } b() // 'b' b(null) // null b(undefined) // 'b' b(false) // false

But I haven't poor much time in this patch, I could be wrong.

You're right that null won't trigger the default; but i'm confused why you'd ever pass in a null in the first place.

i'm confused why you'd ever pass in a null in the first place.

Good question, I have no idea. I was curious to see how React would behave, turns out, they explicitly support it for some reason I'm not aware of.
https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L179

I've asked for the motivation here

ljharb · 2018-08-18T16:55:58Z

packages/enzyme/src/ShallowWrapper.js

@@ -399,11 +399,11 @@ class ShallowWrapper {
   * @param {Function} cb - callback function
   * @returns {ShallowWrapper}
   */
-  setProps(props, callback = noop) {
+  setProps(props, callback) {


either way, the default is required, so the .length of the function is correct.

oliviertassinari · 2018-08-18T17:01:07Z

enzyme is a test framework - its job is to catch bugs

How is it a bug if it's working in production?

Do you have a legitimate use case for passing a falsy non-function to either setState or setProps?

I believe I have, the callback parameter is optional in my case, it can be undefined:
https://github.com/mui-org/material-ui/blob/a29c7f1a7ee885b70159856212206e99b8e86e56/packages/material-ui/src/ButtonBase/TouchRipple.js#L248

ljharb · 2018-08-18T17:03:40Z

If you pass undefined with the current code, it will not throw - the default argument will replace it.

ljharb · 2018-08-18T17:04:45Z

It's a bug because it's unintentional - that something happens to work doesn't make it not a bug.

If you're passing "not a callback", and "not nothing", where a callback is expected, you made a mistake, plain and simple.

oliviertassinari · 2018-08-18T17:07:12Z

If you pass undefined with the current code, it will not throw

If you pass undefined with the current code, it will throw. arguments.length > 1 will make sure of that. It's how I have discovered the issue.
https://github.com/airbnb/enzyme/blob/3bc5de22f8b027fd6db53ac9ac3e8e5e517e6d3a/packages/enzyme/src/ShallowWrapper.js#L434

ljharb · 2018-08-18T17:10:40Z

Ah, you're right, I added that to setState (but not setProps).

That stricter check, however, has already caught a bug in someone's code - see #1769. I think that in this case the benefits outweigh the deviation from React in production. (Separately, I'm going to try to pursue adding the same strict check in react itself in v17)

ljharb · 2018-08-18T17:12:11Z

I do see, however, that because of recent changes, the wrapper's setState will be called in your component code because we stub the instance's setState. That does seem like something we could fix, in the ShallowWrapper constructor - which would allow your code to work - but it would prevent people from being able to catch bugs like in #1769.

oliviertassinari · 2018-08-18T17:17:42Z

Separately, I'm going to try to pursue adding the same strict check in react itself in v17

@ljharb My main purpose with this pull request was to illustrate the change of behavior. I have already patched the Material-UI code to always have a callback function defined. Personally, I would stick to React behavior as much as possible. But I can understand you have a different vision. Do what you think is best.

ljharb · 2018-08-20T06:13:35Z

Per the discussion; I'm going to address this by leaving the wrapper methods strict, but by making the shallow setState stub match React. This way, your code won't error, but people directly calling the methods on the wrapper will get the desired error.

oliviertassinari · 2018-08-20T07:33:17Z

leaving the wrapper methods strict, but by making the shallow setState stub match React.

@ljharb I haven't checked the implementation, but this behavior description sounds great, thanks :)!

oliviertassinari mentioned this pull request Aug 18, 2018

[docs] Add notistack in the related projects mui/material-ui#12578

Merged

ljharb requested changes Aug 18, 2018

View reviewed changes

ljharb closed this in ff11d22 Aug 20, 2018

oliviertassinari deleted the patch-2 branch August 20, 2018 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setState regression #1771

Fix setState regression #1771

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

ljharb left a comment

ljharb Aug 18, 2018

oliviertassinari Aug 18, 2018 •

edited

Loading

ljharb Aug 18, 2018

oliviertassinari Aug 18, 2018 •

edited

Loading

ljharb Aug 19, 2018

ljharb Aug 18, 2018

oliviertassinari commented Aug 18, 2018 •

edited

Loading

ljharb commented Aug 18, 2018

ljharb commented Aug 18, 2018

oliviertassinari commented Aug 18, 2018 •

edited

Loading

ljharb commented Aug 18, 2018

ljharb commented Aug 18, 2018

oliviertassinari commented Aug 18, 2018 •

edited

Loading

ljharb commented Aug 20, 2018

oliviertassinari commented Aug 20, 2018 •

edited

Loading

Fix setState regression #1771

Fix setState regression #1771

Conversation

oliviertassinari commented Aug 18, 2018 • edited Loading

oliviertassinari commented Aug 18, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

ljharb Aug 18, 2018

Choose a reason for hiding this comment

oliviertassinari Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

ljharb Aug 18, 2018

Choose a reason for hiding this comment

oliviertassinari Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

ljharb Aug 19, 2018

Choose a reason for hiding this comment

ljharb Aug 18, 2018

Choose a reason for hiding this comment

oliviertassinari commented Aug 18, 2018 • edited Loading

ljharb commented Aug 18, 2018

ljharb commented Aug 18, 2018

oliviertassinari commented Aug 18, 2018 • edited Loading

ljharb commented Aug 18, 2018

ljharb commented Aug 18, 2018

oliviertassinari commented Aug 18, 2018 • edited Loading

ljharb commented Aug 20, 2018

oliviertassinari commented Aug 20, 2018 • edited Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari Aug 18, 2018 •

edited

Loading

oliviertassinari Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 20, 2018 •

edited

Loading